Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add an email sender policy #91

Merged
merged 2 commits into from
Dec 23, 2020
Merged

add an email sender policy #91

merged 2 commits into from
Dec 23, 2020

Conversation

akash1810
Copy link
Member

What does this change?

Adds a new GuSESSenderPolicy construct which will:

  • Add an EmailSenderAddress parameter to a GuStack
  • Add an IAM policy to allow ses:SendEmail on that address

We have a few stacks that have this permission, each declare the policy in varying ways from no restriction to resource restricted to condition restricted.

After reading this, for our existing use cases it looks like a resource constraint and a condition amount to the same thing? So went with resource constraining as it was first in the docs.

Does this change require changes to existing projects or CDK CLI?

Yes, it simplifies the AMIable stack.

How to test

Run unit tests.

How can we measure success?

Providing more standard constructs and a standard way to declare this policy.

Have we considered potential risks?

n/a

This policy construct will add a parameter to the stack for the sending address, which has regex'd input limiting it to a @theguardian.com email address.
@akash1810 akash1810 added the needs-new-release Identifying significant changes to the library label Dec 23, 2020
@akash1810 akash1810 requested a review from a team December 23, 2020 12:59
Copy link
Contributor

@jorgeazevedo jorgeazevedo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I raise a question about SendRawEmail, but I don't think it's blocking and the rest of the implementation looks really solid

@@ -1,6 +1,6 @@
import type { CfnParameterProps } from "@aws-cdk/core";
import { CfnParameter } from "@aws-cdk/core";
import { Stage, Stages } from "../../constants";
import { RegexPattern, Stage, Stages } from "../../constants";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Encapsulation and unit tests ftw 👍

// see https://docs.aws.amazon.com/ses/latest/DeveloperGuide/sending-authorization-policies.html
new PolicyStatement({
effect: Effect.ALLOW,
actions: ["ses:SendEmail"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember there was one example from the datalake where they had both this and SendRawEmail. I was reading up on the difference in the boto3 docs and SendRawEmail is what you need to use to send an email with an attachment, for example. Do you think it's overkill to include it here as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, was meant to include something about this in the description.

It looks like the datalake's permissions are too wide. That is, it's only calling SendEmailRequest and the permission for SendRawEmail should be dropped as it's not making a SendRawEmailRequest.

We can always add it later if needed.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough!

Effect: "Allow",
Resource: {
"Fn::Join": [
"",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This empty string is a cdk oddity isn't it? I was wondering if there was something to fix on our side, but it looks fine on our side

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's super odd! I'd be reluctant to patch it on our side as this is auto-generated output; any patch we add would need to be compatible with future versions of the aws-cdk library.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, I don't think we should introduce a workaround for this.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We might be able to use one of these functions https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_core.Arn.html.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-new-release Identifying significant changes to the library
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants